ticket/nnnn/sync/stim/aligner #2354
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We are going to need to implement different varieties of StimulusFile sub-classes to represent the other pickle files involved in the VBN data. I did not want any VBO code to accidentally use these new classes.
each has its own way of access num_frames and its own dict_repr key for use in from_json and to_json all inherity from _StimulusFile base class
3 tasks
danielsf
force-pushed
the
ticket/NNNN/sync/stim/aligner
branch
from
082eaf9 to
22c19b4
Compare
danielsf
force-pushed
the
ticket/NNNN/sync/stim/aligner
branch
from
9cc68de to
ee5a593
Compare
This was referenced Apr 8, 2022
morriscb
approved these changes
Apr 8, 2022
| full_msg += f"\nfilepath: {self.filepath}" | ||
| raise RuntimeError(full_msg) | ||
|
|
||
| return None |
There was a problem hiding this comment.
You don't need to explicitly return None as far as I know.
| line=chosen_line, | ||
| units='seconds') | ||
|
|
||
| valid = (falling_edges > rising_edges[0]) |
There was a problem hiding this comment.
Is it guaranteed that these two arrays are the same length?
|
|
||
| # fill lineA and lineB with random bits | ||
| rng = np.random.default_rng(66123) | ||
| indexes = np.arange(0, n_samples, 3) |
There was a problem hiding this comment.
The three here looks to be also used on line 60. I realize this is in a fixture, but if you are use the same value in multiple places (and they need to be changed together if one changes) you should specify it as a variable.
Note: ended up wrapping test clean up code in try/except block the windows CI processes are still getting PermissionErrors cleaning up temporary files. That should not block building/deploying make sure string gets passed to safe_system_path
danielsf
force-pushed
the
ticket/NNNN/sync/stim/aligner
branch
from
ec766ba to
d7ee556
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR ultimately adds the utility function
which is needed to scan a single sync file and find the indices of the starting frames for the behavior_, mapping_, and replay_ stimulus blocks from a single sync file. It is, ultimately, a re-implementation of the method here
https://github.com/AllenInstitute/ecephys_etl_pipelines/blob/main/src/ecephys_etl/modules/vbn_create_stimulus_table/create_stim_table.py#L132-L225
I chose not to make ecephys_etl_pipelines a dependency of the SDK because
I believe we are going to need the flexibility to
get_vsyncsfrom either the rising (in the case of things like running speed or rewards) or falling (in the case of stimulus presentations) edges of the sync file. The ecephys_etl code as implemented does not afford that flexibilityI wasn't sure how to guard against the ecephys_etl implementation changing out from under the SDK unexpectedly.
If it makes anyone feel better, ecephys_etl just copied
sync_dataset.pyfrom the SDK (which copiedsync_dataset.pyfrom some BitBucket repository somewhere).In order to support this work, I had to break up our StimulusFile data objects into three classes.
to represent the three varieties of stimulus file in the VBN experiments. The differences between the three come in how they are represented in our JSON serializations and how
num_framesis accessed.BehaviorStimulusFilecorresponds to theStimulusFileclass that was implemented for the VBO release.The proper use of
get_start_frames_from_stimulus_blocksis something like